Skip to content

C++: Performance fix for FlowVar.getAnAccess#403

Merged
geoffw0 merged 1 commit into
github:masterfrom
jbj:FlowVar-getAnAccess-perf
Nov 5, 2018
Merged

C++: Performance fix for FlowVar.getAnAccess#403
geoffw0 merged 1 commit into
github:masterfrom
jbj:FlowVar-getAnAccess-perf

Conversation

@jbj

@jbj jbj commented Nov 5, 2018

Copy link
Copy Markdown
Contributor

As noted by @geoffw0 in #401, the previous formulation of this predicate caused a CP in snapshots where a variable had a large number of definitions and also reached a large number of sub-basic-blocks.

This should fix performance of https://github.com/FrodeSolheim/fs-uae and https://github.com/libretro/libretro-uae.

The FlowVar.getAnAccess predicate is still at risk of CP'ing when a large group of defs has a large group of uses, but that has not been observed to happen in practice yet. We would need to make localFlowStep expose phi definitions in order to avoid that risk.

@jbj
C++: Performance fix for FlowVar.getAnAccess
9a3907c 
The previous formulation of this predicate caused a CP in snapshots
where a variable had a large number of definitions and also reached a
large number of sub-basic-blocks.

This should fix performance of https://github.com/FrodeSolheim/fs-uae
and https://github.com/libretro/libretro-uae.

The `FlowVar.getAnAccess` predicate is still at risk of CP'ing when a
large group of defs has a large group of uses, but that has not been
observed to happen in practice yet. We would need to make
`localFlowStep` expose phi definitions in order to avoid that risk.
@jbj jbj added the C++ label Nov 5, 2018
@jbj jbj requested a review from geoffw0 November 5, 2018 10:07
@jbj jbj requested a review from a team as a code owner November 5, 2018 10:07
geoffw0
geoffw0 approved these changes Nov 5, 2018
View reviewed changes

@geoffw0 geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My runs of libretro-uae improve from 1049s to 745s with this change (compared to 1337s a few days ago). I'm happy and will merge this assuming the tests have passed.

If we're looking for further improvements there's still one expensive predicate left of the three I originally mentioned on Slack:

inconsistentLoopDirection.ql-11:FlowVar::FlowVar_internal::AlwaysTrueUponEntryLoop::getARelevantVariable_dispred#ff .. 6m30s

@geoffw0 geoffw0 merged commit 5cd7103 into github:master Nov 5, 2018
@jbj

jbj commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

@geoffw0 I can't replicate that performance problem. On libretro-uae I get the following, using the latest master from this repo in QL4E 1.18.0.

	inconsistentLoopDirection.ql-11:FlowVar::FlowVar_internal::AlwaysTrueUponEntryLoop::getARelevantVariable_dispred#ff ....... 455ms

@geoffw0

geoffw0 commented Nov 6, 2018

Copy link
Copy Markdown
Contributor

That's great, we should keep an eye on the query to confirm though.

@geoffw0

geoffw0 commented Nov 30, 2018

Copy link
Copy Markdown
Contributor

For the rc/1.19 branch on my laptop, inconsistentLoopDirection.ql takes 789s on libretro-uae. This PR seems to be working fine but AlwaysTrueUponEntryLoop::getARelevantVariable is still a problem for me:

	inconsistentLoopDirection.ql-11:FlowVar::FlowVar_internal::AlwaysTrueUponEntryLoop::getARelevantVariable_dispred#ff .. 6m36s
	inconsistentLoopDirection.ql-6:ControlFlowGraph::Cached::reachable#f ................................................. 22.1s (executed 19458 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getUpperBoundsImpl#ff ........................................... 12.7s (executed 398 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getLowerBoundsImpl#ff ........................................... 12.2s (executed 347 times)

On ql-master it similarly took 776s with:

	inconsistentLoopDirection.ql-11:FlowVar::FlowVar_internal::AlwaysTrueUponEntryLoop::getARelevantVariable_dispred#ff .. 6m35s
	inconsistentLoopDirection.ql-6:ControlFlowGraph::Cached::reachable#f ................................................. 22s (executed 19458 times)
	inconsistentLoopDirection.ql-6:ConstantExprs::potentiallyReturningFunction#f ......................................... 12.1s (executed 19457 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getUpperBoundsImpl#ff ........................................... 11.8s (executed 398 times)

@jbj

jbj commented Nov 30, 2018

Copy link
Copy Markdown
Contributor Author

Good catch. I'll take a look.

@jbj jbj mentioned this pull request Dec 1, 2018
Merged
@jbj

jbj commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

I should have investigated that problem better when you reported it the first time. The predicate had the shape exists(...) and exists(...), which is always dangerous for the optimiser. I've applied the standard fix in #598.

@geoffw0

geoffw0 commented Dec 3, 2018

Copy link
Copy Markdown
Contributor

With #598 it took just 370s and there are no standout predicates:

	inconsistentLoopDirection.ql-6:ControlFlowGraph::Cached::reachable#f ................................................. 21.5s (executed 19458 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getUpperBoundsImpl#ff ........................................... 12.4s (executed 398 times)
	inconsistentLoopDirection.ql-6:ConstantExprs::potentiallyReturningFunction#f ......................................... 12.2s (executed 19457 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getLowerBoundsImpl#ff ........................................... 11.7s (executed 347 times)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@geoffw0 geoffw0 geoffw0 approved these changes

Assignees

@geoffw0 geoffw0

Labels

C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jbj @geoffw0